{struct,union}: support #[repr(align(...))] and #[repr(packed)] #431

cyphar · 2019-12-13T07:12:01Z

As with #[must_use], this new support requires users to specify what
annotations they would like to apply when an alignment-related repr is
encountered.

In order to facilitate the fact that #[repr(align(...))] takes an
argument which will likely need to be included in the final annotation,
the user must specify a function-like macro which takes the alignment
value as an argument.

It's not clear to me how we should handle aligned enums, so I've
punted on those for now.

Fixes: #430
Signed-off-by: Aleksa Sarai cyphar@cyphar.com

The equality checks for Repr to effectively check ReprStyle didn't make much sense -- it would result in Repr comparisons failing if ReprType was set. This becomes important when we add alignment information to Repr. Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>

emilio

Thanks, this is a great start! I need to take a closer look but looks great off-hand modulo my comments.

We should also add some documentation to docs.md.

src/bindgen/ir/enumeration.rs

src/bindgen/ir/union.rs

tests/rust/aligned_struct.toml

tests/rust/aligned_struct.rs

src/bindgen/ir/enumeration.rs

tests/rust/aligned_struct.rs

cyphar · 2019-12-14T14:13:35Z

The changes you requested have been implemented, but I still have one remaining question (which appears to be a bit difficult to implement with the current way the parser is set up -- the parser doesn't know the full configuration of cbindgen).

Arguably it's not really correct to generate a non-opaque version of a structure without including its alignment (and especially if we don't include its packing) because that will just cause lots of problems when interacting between the C and Rust code. So ideally I think we should treat structs that have alignment as opaque if there isn't a matching alignment macro defined in the configuration. WDYT?

I implemented the above semantics. It wasn't too bad, but I've kept them in a separate commit to hopefully make it easier to review (and easier to revert if you don't like the way it was implemented).

I've implemented #[repr(packed)] and #[repr(align(...))] support in cbindgen[1], and the new binary no longer requires the hacks I had to previously apply. [1]: mozilla/cbindgen#431 Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>

emilio

This looks awesome, thanks!

Just some minor nits and questions about whether there's a use-case for the two different configs. If not I think just using the struct config would be fine, the same way we use the struct.derive_* config for enums that happen to be represented like unions, or the config.structure.rename_fields for unions already.

This looks good to me with those addressed, or if you don't have the time for that I can also do it after landing, your call :)

Thanks for this, and for splitting your commits in a way that it was a breeze to review :)

src/bindgen/ir/repr.rs

tests/rust/aligned.toml

src/bindgen/config.rs

emilio · 2019-12-14T22:14:38Z

And to elaborate: the unfortunate bit that makes must_use weirder (I hope) than this, is that C compilers have different attributes for functions / enum / structs. In particular __attribute__((warn_unused_result)) works for functions, __attribute__((warn_unused)) works for structs / unions, and at least in the past we needed custom annotations/static analysis for enums.

cyphar · 2019-12-15T01:36:31Z

My one follow-up question is are you sure that you want to re-use the struct annotations instead of defining them globally? I'd find it a bit odd (as a user) that a field set in [struct] will affect unions (and eventually enums).

emilio · 2019-12-15T01:46:12Z

My one follow-up question is are you sure that you want to re-use the struct annotations instead of defining them globally? I'd find it a bit odd (as a user) that a field set in [struct] will affect unions (and eventually enums).

I don't know, I think it'd be a bit unfortunate to have to define the same thing three times to get packed / aligned structs to show up right...

On the other hand I see your point. I guess it depends on whether you see struct as "thing that generates a struct" (in which case rust ADTs will fall into it too, for example) or just as "rust struct".

Maybe there should be an [attributes] (or [layout]) section for these? Anyhow given the precedent, probably it is fine to just use struct for now, unless you feel strongly against it. I'd prefer if stuff just worked instead of having to duplicate the same configuration 3 times.

cyphar · 2019-12-15T02:09:18Z

Sorry, I should clarify -- what I meant was that I'm on-board to not having the same config option multiple times. My question was whether there was a reason to not put it in the main Config struct (or as you mentioned, a separate [layout] section)? I'd prefer to do the latter (either one), but if you think it makes sense I'll put it in [struct] (my usecase only involves structs anyway 😉 ).

emilio · 2019-12-15T03:26:37Z

I'm totally fine making it a separate section of the configuration, it's your call :)

While #[repr(packed)] matches the rest of the currently-implemented reprs, #[repr(align(...))] adds another level of nesting and so it's necessary to handle arguments for nested #[repr(...)]s. However, arguments are only permitted for #[repr(align(...))] and any further nesting is ignored as before. It should be noted that #[repr(packed(...))] is also valid in Rust, but due to some complications with translating it to C, it is currently unsupported. Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>

As with #[must_use], this new support requires users to specify what annotations they would like to apply when an alignment-related repr is encountered. However, unlike #[must_use] all compilers appear to use the same attributes to indicate layout settings so there is just a single configuration for this new feature in a separate [layout] TOML table. In order to facilitate the fact that #[repr(align(...))] takes an argument which will likely need to be included in the final annotation, the user must specify a function-like macro which takes the alignment value as an argument. Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>

Users configure the annotations for this feature using the same [layout] settings as for structures (because -- unlike #[must_use] -- all well-known compilers use the same annotations for all types). Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>

If the user has not specified a corresponding annotation for types that carry #[repr(packed)] or #[repr(align(...))] we must not generate the layout because it can cause interoperability issues (and possible memory unsafety) between C and Rust code. This unfortunately required some less-than-pretty plumbing changes so that the load phase of the parser has access to the complete Config object (luckily this does make the overall code look a bit cleaner IMHO). The individual IR loaders only get their corresponding structs, to avoid introducing confusing semantics. Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>

The tests are very straightforward, and effecitvely are just ensuring that the formatting works correctly and is included in all of the important cases. It's also very important to ensure we do not generate laid-out structs for layouts which we cannot reasonably represent in C (such as in cases where we weren't told what annotation to use for packed and specifically-aligned structures). Thus, add some tests to verify that this is the case. Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>

I've implemented #[repr(packed)] and #[repr(align(...))] support in cbindgen[1], and the new binary no longer requires the hacks I had to previously apply. [1]: mozilla/cbindgen#431 Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>

cyphar · 2019-12-15T08:09:21Z

There, all the relevant changes have been made.

Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>

emilio

Looks great, thank you!

emilio · 2019-12-15T10:52:54Z

Let me know if you need a release with these changes and I can try to prepare one soon-ish.

* Added support for #[repr(align)] and #[repr(packed)] on structs and unions. #431 * Added support to generate copy-assignment operators for tagged enums. #434

157: Bump cbindgen from 0.11.1 to 0.12.0 r=MikailBag a=dependabot-preview[bot] Bumps [cbindgen](https://github.com/eqrion/cbindgen) from 0.11.1 to 0.12.0. <details> <summary>Changelog</summary> <p><em>Sourced from <a href="https://github.com/eqrion/cbindgen/blob/master/CHANGES">cbindgen's changelog</a>.</em></p> <blockquote> <h2>0.12.0</h2> <pre><code> * Added support for #[repr(align)] and #[repr(packed)] on structs and unions. mozilla/cbindgen#431 * Added support to generate copy-assignment operators for enums. mozilla/cbindgen#434 </code></pre> </blockquote> </details> <details> <summary>Commits</summary> <ul> <li><a href="https://github.com/eqrion/cbindgen/commit/38fda6b7785a09721d45ed723b14b118b46ae711"><code>38fda6b</code></a> v0.12.0</li> <li><a href="https://github.com/eqrion/cbindgen/commit/48f1038918b7c34bef291fb8136ce88493bbc48d"><code>48f1038</code></a> enum: Add an option to generate copy-assignments.</li> <li><a href="https://github.com/eqrion/cbindgen/commit/2b2ab904cc13a65b73e52d84541f3181f7eea858"><code>2b2ab90</code></a> docs: add documentation for new alignment config knobs</li> <li><a href="https://github.com/eqrion/cbindgen/commit/f23d4ee6e8cf834deb2ef503cccb9da80b789c45"><code>f23d4ee</code></a> tests: add tests for #[repr(packed)] and #[repr(align(...))]</li> <li><a href="https://github.com/eqrion/cbindgen/commit/725378d4199f766b5ecfe692176ce0016ea1cc58"><code>725378d</code></a> {struct,union}: do not generate unsafe layouts</li> <li><a href="https://github.com/eqrion/cbindgen/commit/b95df2bd2bdc309ff3347a4ff1be2cfb38f03d7c"><code>b95df2b</code></a> ir: union: support #[repr(align(...))] and #[repr(packed)]</li> <li><a href="https://github.com/eqrion/cbindgen/commit/89781e204bdcf52e4eadba30b00009c615fc4fe3"><code>89781e2</code></a> ir: struct: support #[repr(align(...))] and #[repr(packed)]</li> <li><a href="https://github.com/eqrion/cbindgen/commit/377d93ec888576b51c6ff321ef7fa85a0d15a82d"><code>377d93e</code></a> ir: repr: support #[repr(align(...))] and #[repr(packed)] parsing</li> <li><a href="https://github.com/eqrion/cbindgen/commit/19e685c87f4ff966da06ef144f955923a8a69f85"><code>19e685c</code></a> ir: {enum,struct,union}: check ReprStyle correctly</li> <li>See full diff in <a href="https://github.com/eqrion/cbindgen/compare/v0.11.1...v0.12.0">compare view</a></li> </ul> </details> <br /> [![Dependabot compatibility score](https://api.dependabot.com/badges/compatibility_score?dependency-name=cbindgen&package-manager=cargo&previous-version=0.11.1&new-version=0.12.0)](https://dependabot.com/compatibility-score.html?dependency-name=cbindgen&package-manager=cargo&previous-version=0.11.1&new-version=0.12.0) Dependabot will resolve any conflicts with this PR as long as you don't alter it yourself. You can also trigger a rebase manually by commenting `@dependabot rebase`. [//]: # (dependabot-automerge-start) [//]: # (dependabot-automerge-end) --- <details> <summary>Dependabot commands and options</summary> <br /> You can trigger Dependabot actions by commenting on this PR: - `@dependabot rebase` will rebase this PR - `@dependabot recreate` will recreate this PR, overwriting any edits that have been made to it - `@dependabot merge` will merge this PR after your CI passes on it - `@dependabot squash and merge` will squash and merge this PR after your CI passes on it - `@dependabot cancel merge` will cancel a previously requested merge and block automerging - `@dependabot reopen` will reopen this PR if it is closed - `@dependabot close` will close this PR and stop Dependabot recreating it. You can achieve the same result by closing it manually - `@dependabot ignore this major version` will close this PR and stop Dependabot creating any more for this major version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this minor version` will close this PR and stop Dependabot creating any more for this minor version (unless you reopen the PR or upgrade to it yourself) - `@dependabot ignore this dependency` will close this PR and stop Dependabot creating any more for this dependency (unless you reopen the PR or upgrade to it yourself) - `@dependabot use these labels` will set the current labels as the default for future PRs for this repo and language - `@dependabot use these reviewers` will set the current reviewers as the default for future PRs for this repo and language - `@dependabot use these assignees` will set the current assignees as the default for future PRs for this repo and language - `@dependabot use this milestone` will set the current milestone as the default for future PRs for this repo and language - `@dependabot badge me` will comment on this PR with code to add a "Dependabot enabled" badge to your readme Additionally, you can set the following in your Dependabot [dashboard](https://app.dependabot.com): - Update frequency (including time of day and day of week) - Pull request limits (per update run and/or open at any time) - Out-of-range updates (receive only lockfile updates, if desired) - Security updates (receive only security updates, if desired) </details> Co-authored-by: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com>

Changelog: 0.12.1 * Added support for #[repr*64)] on enums. mozilla/cbindgen#441 * Added support to generate plain enums instead of enum classes for C++. mozilla/cbindgen#443 * Fixed dependency resolution with lockfile v2. mozilla/cbindgen#438 0.12.0 * Added support for #[repr(align)] and #[repr(packed)] on structs and unions. mozilla/cbindgen#431 * Added support to generate copy-assignment operators for enums. mozilla/cbindgen#434 0.11.1 Not available 0.11.0 * Made rust char map to uint32_t. mozilla/cbindgen#424

emilio requested changes Dec 13, 2019

View reviewed changes

This was referenced Dec 14, 2019

#[repr(align(n))] enum support #432

Open

#[repr(packed(n))] struct support #433

Open

cyphar changed the title ~~ir: struct: support #[repr(align(...))] and #[repr(packed)]~~ {struct,union}: support #[repr(align(...))] and #[repr(packed)] Dec 14, 2019

cyphar requested a review from emilio December 14, 2019 14:18

emilio approved these changes Dec 14, 2019

View reviewed changes

cyphar added 5 commits December 15, 2019 17:58

docs: add documentation for new alignment config knobs

dda0d00

Signed-off-by: Aleksa Sarai <cyphar@cyphar.com>

emilio approved these changes Dec 15, 2019

View reviewed changes

emilio merged commit 2b2ab90 into mozilla:master Dec 15, 2019

cyphar deleted the repr-struct-alignment branch December 15, 2019 10:52

emilio added a commit that referenced this pull request Dec 16, 2019

v0.12.0

38fda6b

* Added support for #[repr(align)] and #[repr(packed)] on structs and unions. #431 * Added support to generate copy-assignment operators for tagged enums. #434

Sewer56 mentioned this pull request Jul 21, 2023

Templates in Docs & Quick Start both Provide an Incomplete cbindgen.toml #858

Open

emilio mentioned this pull request May 14, 2024

Generate C packed struct #952

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

{struct,union}: support #[repr(align(...))] and #[repr(packed)] #431

{struct,union}: support #[repr(align(...))] and #[repr(packed)] #431

cyphar commented Dec 13, 2019 •

edited

Loading

emilio left a comment

cyphar commented Dec 14, 2019 •

edited

Loading

emilio left a comment

emilio commented Dec 14, 2019

cyphar commented Dec 15, 2019

emilio commented Dec 15, 2019

cyphar commented Dec 15, 2019 •

edited

Loading

emilio commented Dec 15, 2019

cyphar commented Dec 15, 2019

emilio left a comment

emilio commented Dec 15, 2019

{struct,union}: support #[repr(align(...))] and #[repr(packed)] #431

{struct,union}: support #[repr(align(...))] and #[repr(packed)] #431

Conversation

cyphar commented Dec 13, 2019 • edited Loading

emilio left a comment

Choose a reason for hiding this comment

cyphar commented Dec 14, 2019 • edited Loading

emilio left a comment

Choose a reason for hiding this comment

emilio commented Dec 14, 2019

cyphar commented Dec 15, 2019

emilio commented Dec 15, 2019

cyphar commented Dec 15, 2019 • edited Loading

emilio commented Dec 15, 2019

cyphar commented Dec 15, 2019

emilio left a comment

Choose a reason for hiding this comment

emilio commented Dec 15, 2019

cyphar commented Dec 13, 2019 •

edited

Loading

cyphar commented Dec 14, 2019 •

edited

Loading

cyphar commented Dec 15, 2019 •

edited

Loading